Skip to content

Conversation

@penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Dec 23, 2025

Closes #49 (we can't always use views but this PR does so where possible; see the _maybe_view function for details)
Closes #136 (by removing the methods)
Closes #148 (note that we can't fully remove the dependency on Accessors.jl, not without reimplementing a ton of stuff, but we have removed the dependency on its types)

This PR modifies VarName to use in-house optics, instead of the optics from Accessors.jl. See the HISTORY.md for details. This PR also adds proper documentation, which you can read at https://turinglang.org/AbstractPPL.jl/previews/PR150/varname/.

I'm aware that this is practically impossible to review, but if you are seeing this, you can take heart from the fact that pretty much all the pre-existing tests for high-level functions like prefix, unprefix, varname_leaves, hasvalue, getvalue, have not been touched, and continue to pass. The only cases where I had to change the tests were those where the actual semantics were changed, e.g. begin is now no longer automatically concretised. (In fact, this PR also expands the test suite by quite a bit.) Besides, DynamicPPL CI passes just fine with only a handful of interface changes, and benchmarks don't show any performance regressions: TuringLang/DynamicPPL.jl#1185

Essentially, I think it's fair to say that once you have constructed a @varname(...) it will pretty much behave the same way it used to, for the most part you just need to change the types so Iden instead of typeof(identity), etc.


Of course, this is no longer true if you are digging into getoptic(vn) and things like that. The data structure changes are all inside src/varname/optic.jl and src/varname/varname.jl. It looks complicated, but that's because I have made it very general: it's more general than old VarName used to be (begin/end no longer need to be concretised so early), and also more general than Accessors.IndexLens is (it accepts keyword arguments). The reason for this is because, if we are undertaking a big refactoring, we may as well do it correctly. Otherwise in the future if we want Turing to work with other array types (looking at DimArray in particular) we will have to come back and fix it again.

Note that keyword argument support for getindex/setindex! can be a bit patchy, but:

  1. These can (probably) be added as non-breaking patches over time. For example, I think varname_and_value_leaves might need patches to make it work nicely, which I haven't implemented.
  2. Sometimes it's not our fault, the Julia ecosystem just hasn't caught up. For example, BangBang.setindex!! will error if you try to use keyword arguments (so in here I've defaulted to using Base.setindex!). In fact, if anything, I think it's good that we find cases like these to push the ecosystem forward.

@github-actions
Copy link
Contributor

AbstractPPL.jl documentation for PR #150 is available at:
https://TuringLang.github.io/AbstractPPL.jl/previews/PR150/

@codecov
Copy link

codecov bot commented Dec 23, 2025

Codecov Report

❌ Patch coverage is 85.94595% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.32%. Comparing base (0086beb) to head (2571d7d).

Files with missing lines Patch % Lines
src/varname/optic.jl 87.80% 25 Missing ⚠️
src/varname/varname.jl 84.50% 11 Missing ⚠️
src/varname/prefix.jl 0.00% 9 Missing ⚠️
src/varname/hasvalue.jl 81.48% 5 Missing ⚠️
src/varname/leaves.jl 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
- Coverage   86.40%   84.32%   -2.08%     
==========================================
  Files           9       10       +1     
  Lines         456      555      +99     
==========================================
+ Hits          394      468      +74     
- Misses         62       87      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@penelopeysm penelopeysm changed the title [WIP] VarName rework VarName rework Dec 24, 2025
@penelopeysm penelopeysm marked this pull request as ready for review December 24, 2025 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Drop Accessors dependency Fix method ambiguities Use views in concretization

2 participants